merge polymer changes #3
Conversation
WalkthroughThis update primarily introduces a new GitHub Actions workflow for Docker image management, enhances testing infrastructure with new utilities and skipping mechanisms for unsupported Ethereum methods, and adds new functionalities across various components for better handling of Ethereum operations. It also improves code reusability and readability in test handling and introduces placeholders for future verifications in Ethereum-related operations. Changes
This table groups files with similar changes together, providing a concise overview of the modifications across the project. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -228,6 +225,7 @@ func startConfigWithTestAccounts(t *testing.T, cfg *SystemConfig, accountsToGene | |||
| // TestMixedDepositValidity makes a number of deposit transactions, some which will succeed in transferring value, | |||
| // while others do not. It ensures that the expected nonces/balances match after several interactions. | |||
| func TestMixedDepositValidity(t *testing.T) { | |||
| t.Skip("fails with: the method eth_getTransactionCount does not exist/is not available") | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Review Summary
The review comment suggests that the use of t.Skip in the TestMixedDepositValidity function is justified due to the unavailability of the eth_getTransactionCount method in the Ethereum client used in the project. The comment also suggests verifying the availability of this method in the future.
Analysis of Shell Script Output
The output from the executed shell script indicates that the eth_getTransactionCount method is indeed referenced in multiple files within the project:
- proxyd/rewriter_test.go and proxyd/rewriter.go: The method is mentioned as part of tests and rewriter configurations.
- op-service/txmgr/txmgr.go: Comments in this file discuss fetching nonce using
eth_getTransactionCount. - op-service/client/client.go: The method is used in functions that seem to be instrumented for metrics or logging.
Despite these references, the specific tests in op-e2e/system_tob_test.go and op-e2e/deposit_test.go have been skipped with a note stating that the method does not exist or is not available. This discrepancy suggests that while the method is generally recognized and utilized in the codebase, there might be specific contexts or configurations in which it is unavailable or not functioning as expected.
Conclusion
Given the evidence from the codebase, the method eth_getTransactionCount is indeed used in several parts of the project. However, the skips in the test files suggest a possible issue with the method's availability or functionality under certain test conditions or configurations. This could be due to environmental differences, configuration issues, or limitations in the testing setup.
Skip Justified Due to Potential Configuration or Environmental Issues Affecting eth_getTransactionCount
Analysis chain
Skip justified due to unavailable Ethereum method eth_getTransactionCount.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the availability of the `eth_getTransactionCount` method in the Ethereum client used.
rg --type go 'eth_getTransactionCount'
Length of output: 955
| **Note that the e2e tests expect to find the shim and polymer within the `external_polymer` directory | ||
| and there's no internal hook to automatically build them.** |
There was a problem hiding this comment.
Ensure files end with a newline.
+ \nIt's a common practice to end files with a newline character to conform to POSIX standards and ensure proper processing by Unix tools.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| **Note that the e2e tests expect to find the shim and polymer within the `external_polymer` directory | |
| and there's no internal hook to automatically build them.** | |
| **Note that the e2e tests expect to find the shim and polymer within the `external_polymer` directory | |
| and there's no internal hook to automatically build them.** | |
op-service/eth/account_proof.go
Outdated
| // TODO polymer does not support valid storage proofs yet, so do a light verification | ||
| func (res *AccountResult) Verify(stateRoot common.Hash) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Placeholder method for account verification.
The Verify_IGNORED method currently returns nil without performing any verification. This could lead to security vulnerabilities if not addressed before production. Consider implementing the method fully or removing it if it's not needed.
* update tests to conform with Polymer * add roothash to L1BlockInfo and ignore storage proofs * adding support for marshalling/unmarshalling blocks * Github workflow build and publish docker images --------- Co-authored-by: jlafiandra6 <[email protected]> Co-authored-by: inkvi <[email protected]>
No description provided.